Add kernelCTF CVE-2023-4244_mitigation #161
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
koczkatamas
force-pushed
the
master
branch
3 times, most recently
from
933028d to
c111d81
Compare
koczkatamas
force-pushed
the
master
branch
13 times, most recently
from
76d6b57 to
bf0d68f
Compare
There was a problem hiding this comment.
Hey,
This is just a quick code quality review. We're planning to review the submissions more deeply (actually understanding what the exploit does) next week.
In general, the code quality looks good, no major concerns. I've left a few smaller comments.
We also have a draft style guide now. Please take a look and let us know if it's helpful for understanding our code quality expectations: https://google.github.io/security-research/kernelctf/style_guide.
Thanks for the submission and PR!
| struct nlmsghdr * nlh; | ||
| struct mnl_nlmsg_batch * batch; | ||
| struct mnl_socket * nl; | ||
| uint32_t portid; |
There was a problem hiding this comment.
This variable is only set, but never used again, can we remove it if it is not necessary?
See our (draft) style guide for more details.
| uint64_t kbase = 0; | ||
|
|
||
| char* buf; | ||
| struct nlmsghdr * nlh; |
There was a problem hiding this comment.
It seems that these variables could've been locals instead of global ones. Our rationale why moving these into locals can improve readability is in our style guide.
| struct mnl_nlmsg_batch * batch; | ||
| struct mnl_socket * nl; | ||
| uint32_t portid; | ||
| uint8_t family = NFPROTO_IPV4; |
There was a problem hiding this comment.
Why do we need to use a variable for this and not use NFPROTO_IPV4 directly in the code? If we want to make this configurable then we prefer a #define or const variable, making clear that this variable won't change during the exploit (so it's a constant).
| char * chain_base_name = "chain1"; | ||
| char * chain_rop_name = "chain_rop"; | ||
|
|
||
| size_t KERNEL_BASE = 0; |
There was a problem hiding this comment.
This variable is unused, please remove it: https://google.github.io/security-research/kernelctf/style_guide#unused-code
| nftnl_set_set_u32(set, NFTNL_SET_GC_INTERVAL, 1); | ||
| nftnl_set_set_u32(set, NFTNL_SET_DATA_TYPE, NFT_DATA_VERDICT); | ||
|
|
||
| uint8_t key_data[0x200] = {0,}; |
There was a problem hiding this comment.
Does the size of key_data matter (0x200)? Does this influence e.g. which cache is used? Please comment here details: https://google.github.io/security-research/kernelctf/style_guide#name-andor-comment-numeric-constants
| nftnl_set_set_u32(set, NFTNL_SET_DATA_TYPE, NFT_DATA_VERDICT); | ||
|
|
||
| uint8_t key_data[0x200] = {0,}; | ||
| uint8_t key_end_data[0x2000] = {0,}; |
There was a problem hiding this comment.
The variables key_end_data and user_data are not used, please remove them: https://google.github.io/security-research/kernelctf/style_guide#unused-code
|
Also please sign the CLA with your other email address too. Currently the CLA check fails. |
No description provided.